Disregard the failed unit tests, that was due to multiple AddCommands and I was lost in the weeds thinking I broke something with that property comparer. Those are fixed, but I'm still convinced I broke something with the comparer, but I'm not sure what...

Awesome work. Now comes the big question, how do we know what to update when we make changes and/or additions. Otherwise the trimming support will slowly become more and more unusable. Could we document this somewhere?

MS confirmed the bug with NativeAOT. I cleaned up the commit to be explicit about why there is a work around, separate from the main AOT work.

Regarding the future of maintenance once this is enabled, with <IsTrimmable>true</IsTrimmable> set, the compiler will alert us to what we need to do moving forward. If you add code that uses reflection, it'll warn you to make sure you mark your code appropriately with the DynamicallyAccessedMembers attribute. Really the only way things could get screwy is if someone went into a method marked to suppress warnings and started using reflection on types we hadn't considered yet. A scenario like that is FlagValue where it is never instantiated directly so its constructor was getting trimmed out.

The biggest maintenance headache is trying to read the code, especially the generic argument definitions, with the #IF statements to work around NetStandard support.

But give me a bit to grab a beer and I'll add additional comments throughout, especially around FlagValue and where we've suppressed messages.

@phil-scott-78 Sounds good to me!

My recommendation for making the attributes cleaner is using the polysharp nuget package and switch on the flag to generate runtime attributes.

I’ve also been working on annotation so I’ll take a look at this later. I’ll say that I’m pretty skeptical that CLI can be made safe without a source generator.

Missed this

Added a new method for adding commands that allows explicit configuration of the settings to prevent them from being trimmed.

yeah that sounds right. Automatic config probably doesn’t work, but manual should be fine.

Love the idea of polysharp.

I'm sold on polysharp. Added it and reworked the PR to use it.

If we're going to use PolySharp, we should probably remove the other polyfill libraries that we use.

Also, might be a good idea to create a new demo where trimming is enabled and leave the current demo code as-is.

The final four games were pretty boring last night, and the boys went to bed in a reasonable manner, so I had time to implement @agocke's suggestion on polysharp and tweaking some of the attributes. Pushed up those changes.

I'll create a new DemoAOT project. I think that's a good idea. Not only does it keep the original there, having one explicitly with AOT will lead to (hopefully) better discovery.

Added the new DemoAOT project to demonstrate how to configure the project. One hang up right now is the string constructor trick @0xced included a while back where we automatically convert types with a single param string constructor (e.g. DirectoryInfo). I added an example on how to work around that, but it'd be nice if there was a source generator that could produce those required attributes to keep the compiler from stripping those dynamically called constructors out.

@phil-scott-78 Is this ready for review, or do you have more things to add/fix/change?

One more thing to consider is, rather than a demo app, you can use a "test app" to have the linker/aot compiler verify all the dependencies. Instructions are at https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming?pivots=dotnet-8-0#show-all-warnings-with-test-app but the key part is adding TrimmerRootAssembly that causes the trimmer to treat all entry points into a library as being reachable.

Here's the code for an AOT test app that I wrote and put in the test folder:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>

    <PublishAot>true</PublishAot>
    <!-- Show warnings from dependencies, not just from the app -->
    <TrimmerSingleWarn>false</TrimmerSingleWarn>
  </PropertyGroup>

  <ItemGroup>
    <ProjectReference Include="../../src/Spectre.Console.Cli/Spectre.Console.Cli.csproj" />
    <TrimmerRootAssembly Include="Spectre.Console" />
    <TrimmerRootAssembly Include="Spectre.Console.Cli" />
  </ItemGroup>

</Project>

Before review, I definitely want to try out that @agocke test project. I had a few minutes to use a computer today and added it only to find a few more warnings.

I’ve tried to do some annotation on my own. I could share it but don’t know the best way. Comments in this PR? New PR?

I’ve tried to do some annotation on my own. I could share it but don’t know the best way. Comments in this PR? New PR?

If it's a handful of improvements, comments on the PR should work. You could also do a PR to my fork too, I suppose.

Honestly, whatever feedback you can give I'm excited for, so what's easiest for you to communicate it.

Pushed all my changes to https://github.com/agocke/spectre.console/tree/cli-trimmable. There are a lot of CLI annotations, but I'm not sure there's a simple alternative.

@agocke pulled in your code to my project - it looks like we need to add [DynamicallyAccessedMembers] to the ITypeRegistrar. I didn't see this attribute being used already, are we doing this in some other way?

Without the above, trimming doesn't seem to work when integrating with Microsoft.Extensions.DependencyInjection:

public class DependencyInjectionRegistrar(IServiceCollection services) : ITypeRegistrar, IDisposable
{
    // ... omitted code

    public void Register(Type service, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type implementation)
    {
        Services.AddSingleton(service, implementation);
    }

    // ...
}

@codymullins did you also pull the project file changes? Polysharp adds the polyfill for the attributes.

I forked the repo...not familiar with Polysharp. Let me investigate if I forked anything up 😄

edit - yes, looks like I'm the problem here. the build is failing but I'm sure that's also me - I'll retest and update.

Wanted to give an update on where I am with this: I think I've got the annotations in a good place locally, and they make plain Spectre.Console usable, minus the changes to TypeConverter.

Spectre.Console.Cli is another story. The annotations make it unusable -- all the current APIs are not trim safe. Unfortunately, I don't see a way to use the existing APIs to manually configure it, either. Reflection is deeply baked into the API surface.

What I've started is defining a new parallel set of APIs that allow a combination of configuration and reflection, that could eventually be implemented by a source generator.

Right now I'm kind of stuck on Bind. There's both a lot of reflection in there and a lot of currently internal surface area, so lifting out a new API is taking a while.

An intermediate state that I think would be good is just annotating the current surface. That would make it clear what's usable and what's not. And we need to define a path forward for the TypeConverter behavior in Spectre.Console. The existing TypeConverter is a non-starter.

I happened to have a chance to look at this today and I also landed at your idea of the intermediate step. I don't think AOT perfection is possible but given a handful of well-placed annotations it does become quite difficult to break things. Trickiest thing, to me at least, is the magic for trying to call a constructor for a type with a single string param constructor.

Other than that, most cases seem to chug along, right? Intrinsic types have their TypeConverter handled pretty well in .NET 8. Custom types we've always pushed people to annotate their fields with a TypeConverter annotation which has the right annotations itself for including the proper DynamicallyAccessedMemberTypes. Once we get past that, we've never really supported anything overly complex out of the box like List<T> or anything beyond arrays of intrinsic types, which seem to work fine.

Of course, there might be countless other things I'm missing here. Could you provide some examples of things that absolutely would not work with the Cli project because of the current TypeConverter? Even just documenting those might be a good step, and maybe improve the error messages too.

The problem is that debugging AOT-specific problems is basically impossible. Our guidance for AOT-safety is to either guarantee behavior, or mark it as unsafe. Suppressing warnings that then bite users during deployment isn't really doing any favors.

I do think I can build a new API for the CLI layer, it's just going to take a while.

The only other option I can think of is explicitly throwing NotSupported exceptions for unsupported features. But that still means -- avoid suppressions. Either use a feature switch like I have in my branch for type converter, or just straight change the behavior (even in non-trimmed scenarios), to stop supporting complex reflection behavior.

Could you provide some examples of things that absolutely would not work with the Cli project because of the current TypeConverter?

So, the big problem with TypeConverter is just that GetConverter is fundamentally unsafe due to reliance on ICustomTypeDescriptor as a fallback. My alternate implementation works around this by implementing a GetConverter method that only uses the TypeConverter attribute and doesn't respect ICustomTypeDescriptor.

Beyond that we still run into problems in binding. GetConverter is a good example

if (parameter.ParameterType.IsArray)
{
    // Return a converter for each array item (not the whole array)
    var elementType = parameter.ParameterType.GetElementType();
    if (elementType == null)
    {
        throw new InvalidOperationException("Could not get element type");
    }

    return (TypeDescriptor.GetConverter(elementType), GetStringConstructor(elementType));
}

Let's say you replace that GetConverter call with a call to my "safe" version that has DynamicallyAccessedMembers on it. That doesn't help because the annotations don't allow you to annotate array element types, just the array itself. So the return type of parameter.ParameterType.GetElementType() will never satisfy those annotations. What we can do instead is pass along an extra piece of information: as part of the parameter we not only keep the parameter Type, we also keep the element Type. The constructor for CommandParameter would require you to pass both.

The trick for this is it just becomes very verbose for the user, and really removes the benefit of doing reflection at all. The user ends up typing a bunch of boilerplate code. Hence the source generator.

Setting aside 20 years of .Net coding and still only being able to partially follow the above discussion....

I'm missing the use case for AOT in this scenario @agocke (@phil-scott-78) - who would want to trim Spectre.Console and for what purpose, and on what platforms?

Performance tweaks for a CLI application don't make sense to me, so I'm left with native compiled .Net apps on Linux/Mac, avoiding the need to install (then maintain) the .Net SDK.

Am I broadly correct in my guess work?

A wider question is the suitability of AOT for prime time? The above discussion seems fraught with code massaging unfamiliar to standard .Net development practices.

Interested in where this might go.

Are you using native AOT? What is your experience?
https://www.reddit.com/r/dotnet/comments/1d6lfjh/are_you_using_native_aot_what_is_your_experience/?rdt=46863

I use Spectre.Console in my Native AOT CLI I distribute at work. It's actually a perfect use-case for AOT, you typically want AOT for apps that need to start fast, a non-AOT CLI feels sluggish.

I use Spectre.Console in my Native AOT CLI I distribute at work. It's actually a perfect use-case for AOT, you typically want AOT for apps that need to start fast, a non-AOT CLI feels sluggish.

Same here. Startup time without AOT can easily be more than a second and with AOT it can reach a 1/10th of that, makes the CLI feel a lot more responsive. I also tested making another CLI with another purpose using System.CommandLine which does support trimming fully and then I was able to make a complete self-contained AOT binary in just a few MB while my Spectre.Console AOT CLI is more like ~25MB if I remember correctly. It would be very nice if I could do the same which Spectre.Console as I liked it a lot more apart from not having full trim support. A smaller binary should reduce startup times even more because then there's much less data that needs to be read from disk when first starting up.

Thank you @slang25, @AnonymousRetard for your comments - I'm obviously not well-informed about AOT, so it's good to hear actual user feedback.

I'll add my two-sense, I got interested in this when I decided to see if I could make an oh-my-posh like prompt in .Net. I don't have my results handy, but my "time to first rendering of some yellow text and a smiley face" (TTFROSYTASF) benchmark started at like 140ms and dropped to something below 40ms. That's a huge difference for that usage scenario.

Now, the question as to whether it is worth the developer headache: My personal opinion right now would be to put the effort in the main Spectre.Console library to make it compatible, but then mark the Spectre.Console.Cli project as explicitly not supported due to reflection with a RequiresDynamicCode attribute on the app.Run methods and be done with it. I'm not sure that project can ever support it without some very involved refactoring

dropping this effort for a new one...

A good effort @phil-scott-78, none the less.